feat: add option to omit query string from path matching and logging #110
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #110 +/- ##
==========================================
- Coverage 94.24% 90.53% -3.72%
==========================================
Files 2 2
Lines 139 169 +30
==========================================
+ Hits 131 153 +22
- Misses 5 14 +9
+ Partials 3 2 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Please resolve the conflicts. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds an option to omit the query string from the path for logging and matching, with corresponding changes in tests, logger behavior, examples, and documentation.
- Added a new Option function (WithPathNoQuery) to enable query-string omission.
- Updated logging behavior in logger.go to conditionally separate the query string into its own field.
- Extended tests and examples in logger_test.go, _example/main.go, and README.md to demonstrate this new functionality.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| options.go | Introduces WithPathNoQuery to set a flag that controls query string inclusion in paths. |
| logger_test.go | Adds tests to verify the behavior when the query string is separated from the logged path. |
| logger.go | Updates the logging implementation to conditionally omit the query string from the path. |
| _example/main.go | Provides an example usage of the WithPathNoQuery option. |
| README.md | Updates documentation with an example of using WithPathNoQuery. |
Comments suppressed due to low confidence (1)
options.go:273
- [nitpick] Consider renaming the boolean parameter 'b' to a more descriptive name, such as 'enabled', to improve code clarity.
func WithPathNoQuery(b bool) Option {
|
@appleboy done! |
There was a problem hiding this comment.
Pull Request Overview
Adds a new option to strip query strings from the logged path and match rules, emitting the query separately when enabled.
- Introduce
WithPathNoQueryconfig flag and option constructor - Update
SetLoggerto conditionally remove query frompathand add aqueryfield - Add tests, example code, and README entries for the new behavior
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| options.go | Define WithPathNoQuery option to toggle query stripping |
| logger.go | Update path construction and log event to honor pathNoQuery |
| logger_test.go | Add TestLoggerPathNoQuery to validate query handling |
| _example/main.go | Example code demonstrating WithPathNoQuery usage |
| README.md | Document example for WithPathNoQuery in the project README |
Comments suppressed due to low confidence (3)
_example/main.go:177
- [nitpick] The variable name
nqis very short and may be unclear in example code. Consider renaming it to something more descriptive, likenoQueryGroup, to improve readability.
nq := r.Group("/no-query", logger.SetLogger(
README.md:140
- [nitpick] In README examples,
nqis a terse variable name. Using a more descriptive name such asnoQueryGroupwill help readers understand its purpose.
nq := r.Group("/no-query", logger.SetLogger(
logger_test.go:371
- The test checks for the presence of the
queryfield but doesn't verify that thepathfield is free of the query component. Adding an assertion to confirmpathexcludes?foo=barwhenWithPathNoQuery(true)would strengthen coverage.
for i, line := range lines {
| @@ -136,6 +136,23 @@ func main() { | |||
| }) | |||
| } | |||
|
|
|||
| // Example of WithPathNoQuery usage | |||
There was a problem hiding this comment.
[nitpick] Consider adding a sample of the actual logged output after this example block. Showing how path and query fields appear will make the feature's effect more concrete.
Copilot uses AI. Check for mistakes.
| @@ -157,17 +163,22 @@ func SetLogger(opts ...Option) gin.HandlerFunc { | |||
|
|
|||
| start := time.Now() | |||
| path := c.Request.URL.Path | |||
| if raw := c.Request.URL.RawQuery; raw != "" { | |||
| path += "?" + raw | |||
| rawQuery := c.Request.URL.RawQuery | |||
There was a problem hiding this comment.
[nitpick] The logic for extracting and handling the query string is duplicated in two places. Extracting path and query parsing into a small helper function would reduce duplication and improve maintainability.
Copilot uses AI. Check for mistakes.
Adds an option (default disabled for backwards compatibility) to change the handling of the
pathfield to omit the query string.When enabled:
pathis logged without the query stringqueryfieldpathwithout the query string is used for matching in the theWithSkipPath,WithSkipPathRegexps, andWithPathLeveloptionsWhy?
(This previously was #108, but it was closed by accident due to a push to a private repo referencing that PR)